Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new initialization arguments for mask class to support set mask with less code #206

Merged
merged 5 commits into from
Apr 18, 2024

Conversation

w1nda
Copy link
Contributor

@w1nda w1nda commented Apr 8, 2024

When writing a test, for example a DHCP server test, I only care if the IP provided as designed.

However, when I validate the packet by PTF, I need to find all fields and then call method set_do_not_care_scapy multiple times to set all fields' mask to "don't care" except the IP DHCP server provided.

So, I added new initialization arguments for mask class to support set mask with much less effort.

if (exp_pkt[i] & self.mask[i]) != (pkt[i] & self.mask[i]):
return False
return True
return hdr_offset * 8 + offset, bitwidth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the set_do_not_care call here? Shouldn't this be handled by the mask?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't remove the set_do_not_care call, this diff displaying is quite confused and incorrect. I will try to adjust the order of those functions to get a better diff display.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, but reorder doesn't works well..

To help reviewer, I can explain what have I changed.

  1. I added a new arugument dont_care_all for init
  2. I added function called set_care,set_care_packet and set_care_all versus set_do_not_care,set_do_not_care_scapy.

That's all, the diff showing here is quite confused, for example, if you review the function pkt_match, you can see, I didn't change any single character of it, but the diff tell us I have removed the function and wrote a new one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying, the diff makes it difficult to understand what has been changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the confusion comes from adding _calculate_fields_offset_and_bitwidth. If you move that function above set_do_not_care_packet the diff should look better.

However, set_do_not_care_packet now calls into _calculate_fields_offset_and_bitwidth which does not use self.set_do_not_care(hdr_offset * 8 + offset, bitwidth)

@w1nda w1nda requested a review from fruffy April 11, 2024 01:49
@w1nda
Copy link
Contributor Author

w1nda commented Apr 16, 2024

Hi @fruffy, cloud we merge this PR?

Copy link
Contributor

@fruffy fruffy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditionally approving, it looks like this doesn't change default behavior.

@fruffy fruffy requested a review from antoninbas April 16, 2024 13:55
@fruffy
Copy link
Contributor

fruffy commented Apr 18, 2024

@antoninbas Do you have any reservations? Otherwise I will merge this PR end of this week.

@antoninbas
Copy link
Member

@fruffy Thanks for checking. If this looks good to you, you can merge. I just took a very high-level look.

@fruffy fruffy merged commit c554f83 into p4lang:main Apr 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants